Resolve Custom Tool Entrypoint Relative to Working Directory in Runner#5
Merged
Conversation
Prefix discovered tool Entrypoint with "tools" (via filepath.Join) before creating the custom tool so execution from the agent root can find the file. A copy of the discovered tool is modified to avoid mutating the original, then passed to NewCustomTool.
initializ-mk
approved these changes
Feb 23, 2026
Contributor
initializ-mk
left a comment
There was a problem hiding this comment.
Looks good — clean, minimal fix.
Copying dt before mutating the entrypoint avoids side effects on the original slice, and filepath.Join("tools", dt.Entrypoint) correctly resolves the path relative to WorkDir so the agent root can locate the script.
Verified this applies cleanly against the in-flight core/make-skill-modular branch (PR #6) with no conflicts — the changes are in a completely separate code path.
16 tasks
naveen-kurra
pushed a commit
to naveen-kurra/forge
that referenced
this pull request
May 23, 2026
…nitializ#5) Reviewer flagged: when refreshLocked errors, lastMissRefresh is not updated → every subsequent unknown-kid request hammers the IdP again → a flapping IdP becomes a DDoS amplifier (N callers × broken IdP = N IdP hits per request burst). The reviewer was looking at the pre-fix-initializ#1 code. Fix initializ#1 (commit 379d578) addressed this concern through a DIFFERENT mechanism: it added a lastAttempt + lastErr pair and a backoffActive() guard, so any refresh error (not just unknown-kid-after-refresh) suppresses subsequent refresh attempts inside the refetchGrace window. This commit makes the no-amplifier property explicit by: 1. Adding TestJWKS_FailedRefreshDoesNotHammerIdP — fires 50 unknown-kid requests during a backoff window and asserts the JWKS endpoint receives exactly ONE hit (the initial refresh that failed). Then advances time past refetchGrace and asserts the next request triggers EXACTLY ONE retry — and the burst after that retry stays capped at one IdP call again. 2. Adding a doc comment on backoffActive() that names the DDoS-amplifier concern explicitly and references the test that pins the contract, so future maintainers don't accidentally remove the gate. No code behavior change — fix initializ#1 already produced the right behavior; this commit makes the test coverage match. Verification: go test -race ./forge-core/auth/providers/oidc/ — green (incl. new test) full sweep — 39/39 packages pass golangci-lint v2.10.1 — 0 issues
naveen-kurra
pushed a commit
to naveen-kurra/forge
that referenced
this pull request
May 24, 2026
Batch-clearing the "don't block merge" follow-ups from review of PR initializ#79. initializ#1 gcp_iap classifyJWTErr — use jwt v5 sentinels via errors.Is rather than substring matching (library wording shifts across patches; sentinels are public API). Special-case ErrTokenSignatureInvalid to split alg-confusion (→ ErrInvalidToken) from real bad-signature (→ ErrTokenRejected) because golang-jwt wraps both under that one sentinel. Three internal keyFunc message-matches retained — those are strings WE control, not the library's. initializ#2 gcp_iap JWKS merge-on-success — switched j.keys = newKeys to a per-kid merge. A partial-but-valid JWKS response (e.g. one kid accidentally omitted by GCP during rotation) no longer drops kids the stale-grace contract assumes we still have. Worst case is keeping a retired kid in cache; verification still fails naturally for any token signed with the retired private key. initializ#3 azure_ad GraphCache defensive copies — Get returns append([]string(nil), e.groups...) and Put stores a copy of its input. Caller mutating Identity.Groups (the auth.Identity layer treats it as freely mutable) can't poison the cache. initializ#4 forge-cli needsYAMLQuoting numeric edge cases — quote anything that resembles a YAML number (hex 0x, octal 0o, binary 0b, leading-zero "010", scientific 1e10, decimal float 3.14, signed ±N, .inf / .nan in either case). Auth-setting values rarely hit these shapes but the docstring promised "false negatives are bugs" and the Web UI POST path can supply arbitrary strings. Added looksNumeric() helper with separate allHexDigits / allOctalDigits / allBinaryDigits gates. initializ#5 aws_sigv4 identity_cache_test — replaced string(rune(i)) with strconv.Itoa(i). Surrogate code points (0xD800..0xDFFF) all map to U+FFFD, so the eviction-threshold test was silently building ~10 distinct keys instead of 10_001 and the sweep never ran. initializ#6 http.NewRequestWithContext error handling — fixed the two `req, _ := ...` antipatterns in gcp_iap/iap_jwks.go and azure_ad/graph_client.go. Hardcoded URLs make the failure currently unreachable, but errcheck-clean is the discipline. initializ#7 gcp_iap HS256-with-EC-public-key alg-confusion test — pinned the most dangerous attack shape: attacker fetches the verifier's public key from JWKS (open by design), uses raw X/Y bytes as the HMAC "secret", signs an HS256 token. A non-whitelisting verifier would HMAC-verify it. Our keyFunc rejects on alg != "ES256" BEFORE key lookup; this test confirms. Tests added: TestGraphCache_GetReturnsDefensiveCopy, TestGraphCache_PutStoresDefensiveCopy, TestProvider_HS256WithECPublicKeyAsSecret_Rejected. Existing TestProvider_RS256Token_Rejected still passes (alg-confusion still classified as ErrInvalidToken under the new sentinel-based path). 42 packages green, lint + gofmt clean.
initializ-mk
pushed a commit
that referenced
this pull request
May 24, 2026
Batch-clearing the "don't block merge" follow-ups from review of PR #79. #1 gcp_iap classifyJWTErr — use jwt v5 sentinels via errors.Is rather than substring matching (library wording shifts across patches; sentinels are public API). Special-case ErrTokenSignatureInvalid to split alg-confusion (→ ErrInvalidToken) from real bad-signature (→ ErrTokenRejected) because golang-jwt wraps both under that one sentinel. Three internal keyFunc message-matches retained — those are strings WE control, not the library's. #2 gcp_iap JWKS merge-on-success — switched j.keys = newKeys to a per-kid merge. A partial-but-valid JWKS response (e.g. one kid accidentally omitted by GCP during rotation) no longer drops kids the stale-grace contract assumes we still have. Worst case is keeping a retired kid in cache; verification still fails naturally for any token signed with the retired private key. #3 azure_ad GraphCache defensive copies — Get returns append([]string(nil), e.groups...) and Put stores a copy of its input. Caller mutating Identity.Groups (the auth.Identity layer treats it as freely mutable) can't poison the cache. #4 forge-cli needsYAMLQuoting numeric edge cases — quote anything that resembles a YAML number (hex 0x, octal 0o, binary 0b, leading-zero "010", scientific 1e10, decimal float 3.14, signed ±N, .inf / .nan in either case). Auth-setting values rarely hit these shapes but the docstring promised "false negatives are bugs" and the Web UI POST path can supply arbitrary strings. Added looksNumeric() helper with separate allHexDigits / allOctalDigits / allBinaryDigits gates. #5 aws_sigv4 identity_cache_test — replaced string(rune(i)) with strconv.Itoa(i). Surrogate code points (0xD800..0xDFFF) all map to U+FFFD, so the eviction-threshold test was silently building ~10 distinct keys instead of 10_001 and the sweep never ran. #6 http.NewRequestWithContext error handling — fixed the two `req, _ := ...` antipatterns in gcp_iap/iap_jwks.go and azure_ad/graph_client.go. Hardcoded URLs make the failure currently unreachable, but errcheck-clean is the discipline. #7 gcp_iap HS256-with-EC-public-key alg-confusion test — pinned the most dangerous attack shape: attacker fetches the verifier's public key from JWKS (open by design), uses raw X/Y bytes as the HMAC "secret", signs an HS256 token. A non-whitelisting verifier would HMAC-verify it. Our keyFunc rejects on alg != "ES256" BEFORE key lookup; this test confirms. Tests added: TestGraphCache_GetReturnsDefensiveCopy, TestGraphCache_PutStoresDefensiveCopy, TestProvider_HS256WithECPublicKeyAsSecret_Rejected. Existing TestProvider_RS256Token_Rejected still passes (alg-confusion still classified as ErrInvalidToken under the new sentinel-based path). 42 packages green, lint + gofmt clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a small but important fix to how custom tools are registered in the
Runner. The entrypoint for each discovered tool is now made relative to the working directory, ensuring that execution from the agent root finds the correct file."tools"with the tool's entrypoint, preventing path issues when running from the agent root. (forge-cli/runtime/runner.go, forge-cli/runtime/runner.goL133-R136)